MVP: Cost attribution #10269

ying-jeanne · 2024-12-17T21:00:08Z

What this PR does

This is the follow up of #9733,

The PR intent to export extra attributed metrics in distributor and ingester, in order to get sample received, sample discarded and active_series attributed by cost attribution label.

Which issue(s) this PR fixes or relates to

Fixes #

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

docs/sources/mimir/configure/configuration-parameters/index.md

pkg/costattribution/manager.go

pkg/ingester/activeseries/active_series.go

colega · 2024-12-18T13:13:30Z

pkg/ingester/activeseries/active_series.go

@@ -502,6 +525,18 @@ func (s *seriesStripe) remove(ref storage.SeriesRef) {
 	}

 	s.active--
+	if s.cat != nil {
+		if idx == nil {


Same here, we should assume this isn't nil. Just skipping the removal will break the numbers forever.

vendor and update in commit 4706bde

colega · 2024-12-18T13:14:05Z

pkg/ingester/activeseries/active_series_test.go

Please update the active series tracker tests with the costattribution.Tracker, otherwise the new code isn't tested.

addressed in 17b64a9

colega · 2024-12-18T13:20:41Z

pkg/ingester/ingester.go

+		idx, err := db.Head().Index()
+		if err != nil {
+			level.Warn(i.logger).Log("msg", "failed to get the index of the TSDB head", "user", userID, "err", err)
+			idx = nil
+		}


As commented previously, we should never proceed without an index.

If you check the implementation of db.Head().Index() it never returns an error. We have three options here:

Skip tenants if they don't have index: this is the least effort one.

Panic if err is not nil, this is ugly

Update mimir-prometheus to add a MustIndex() IndexReader method that does not return an error, and use that one.

PR in mimir-prometheus grafana/mimir-prometheus#811

vendor and update in commit 4706bde

colega · 2024-12-18T13:21:35Z

pkg/mimir/modules.go

+	if t.Cfg.CostAttributionRegistryPath != "" {
+		reg := prometheus.NewRegistry()
+		var err error
+		t.CostAttributionManager, err = costattribution.NewManager(3*time.Minute, time.Minute, t.Cfg.CostAttributionEvictionInterval, util_log.Logger, t.Overrides, reg)


I think these values should not be hardcoded.

removed unused parameter b27e379

tacole02

Thanks for updating the docs! I left a few suggestions.

docs/sources/mimir/configure/configuration-parameters/index.md

colega · 2024-12-27T12:02:39Z

go.mod

-replace github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20241219104229-b50052711673
+replace github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20241224134504-460b7be5bce8


You should not update mimir-prometheus here. It should be updated in main, and then you merge main into your branch.

I would vendor main when I got it merged, just to make the code compile now.

colega · 2024-12-27T12:03:14Z

pkg/costattribution/manager.go

+	return m.purgeInactiveAttributionsUntil(time.Now().Add(-m.inactiveTimeout).Unix())
+}
+
+func (m *Manager) EnabledForUser(userID string) bool {


I think this doesn't need to be exported.

addressed in commit 679f2cc

colega · 2024-12-27T12:04:43Z

pkg/costattribution/manager.go

+	if tracker, exists = m.trackersByUserID[userID]; exists {
+		return tracker
+	}
+	tracker = newTracker(userID, m.limits.CostAttributionLabels(userID), m.limits.MaxCostAttributionCardinalityPerUser(userID), m.limits.CostAttributionCooldown(userID), m.logger)


Nit, but I would collect all the information needed to build a tracker (all the m.limits.* calls before we take m.mtx.Lock, to spend less time locked.

addressed in commit 679f2cc

pkg/costattribution/manager.go

colega · 2024-12-27T13:03:04Z

pkg/costattribution/tracker.go

+func (t *Tracker) createNewObservation(key []byte, ts int64, activeSeriesIncrement, receivedSampleIncrement, discardedSampleIncrement float64, reason *string) {
+	t.observed[string(key)] = &observation{
+		lastUpdate:         atomic.NewInt64(ts),
+		activeSerie:        *atomic.NewFloat64(activeSeriesIncrement),
+		receivedSample:     *atomic.NewFloat64(receivedSampleIncrement),
+		discardedSample:    map[string]atomic.Float64{},
+		discardedSampleMtx: sync.Mutex{},
+	}
+	if discardedSampleIncrement > 0 && reason != nil {
+		t.observed[string(key)].discardedSampleMtx.Lock()
+		t.observed[string(key)].discardedSample[*reason] = *atomic.NewFloat64(discardedSampleIncrement)
+		t.observed[string(key)].discardedSampleMtx.Unlock()
+	}
+}


Since we're creating a new observation here, I would pass a key string here, allocated once.

I'm not sure if this method isn't allocating 4 strings here (not a big deal, since it's not a hot path, but still..)

addressed in 0287bf6

pkg/costattribution/tracker.go

pkg/distributor/distributor.go

Co-authored-by: Oleg Zaytsev <[email protected]>

ying-jeanne · 2024-12-31T09:04:30Z

pkg/costattribution/tracker.go

+			labelValues[idx] = missingValue
+		}
+	}
+	key := t.hashLabelValues(labelValues)


I'm not entirely convinced that fetching a key as a string every time is the best approach, but it has simplified the code.

Poc: cost attribution proposal 2

e315ebb

ying-jeanne force-pushed the final-cost-attribution branch from 5165a5b to 6f36b5f Compare December 17, 2024 21:07

ying-jeanne changed the title ~~Final cost attribution~~ MVP: Cost attribution Dec 17, 2024

ying-jeanne requested a review from colega December 17, 2024 21:10

ying-jeanne force-pushed the final-cost-attribution branch from 6f36b5f to 077a94a Compare December 17, 2024 22:01

refectory

f04c28f

ying-jeanne force-pushed the final-cost-attribution branch from 077a94a to f04c28f Compare December 17, 2024 22:08

ying-jeanne marked this pull request as ready for review December 17, 2024 22:13

ying-jeanne requested review from tacole02 and a team as code owners December 17, 2024 22:13

ying-jeanne requested a review from seizethedave December 18, 2024 10:20

colega reviewed Dec 18, 2024

View reviewed changes

tacole02 approved these changes Dec 18, 2024

View reviewed changes

ying-jeanne added 17 commits December 19, 2024 14:09

add experimental features in about-versioning.md

2c422d1

change const variable to private

d2eab6b

make timer service

1f39282

rename TrackerForUser to Tracker

9b4337d

use fine locking

1a523e1

add comments explain why we use unchecked collector

f10f787

rename deleteUserTracker to deleteTracker

cc0e939

rename cat in cost attribution package to t or tracker

c020be0

avoid get tracker twice

71e4666

refactor inactiveObservationsForUser

9dd101b

refactor shouldDelete function

7d4ea9a

rename calabels and calabelmap to labels and index

6754666

remove getter and setter of max cardinality and cooldown duration

fffc5b3

rename CompareLabels to hasSameLabels

2cf8c3e

remove the mapping logic since the slices are ordered

f994034

remove unnecessary tracker nil checking

b060c09

fix linting

e35a8d9

ying-jeanne added 5 commits December 26, 2024 16:02

add unittest in active series

17b64a9

copy slice instead

a191044

add test for discarded samples

2bb1845

change small map to slice since it is quicker

ddd507d

remove unused parameter

b27e379

colega reviewed Dec 27, 2024

View reviewed changes

ying-jeanne and others added 17 commits December 27, 2024 15:58

add new parameter

a79fac7

update config file

37901b7

Update pkg/costattribution/manager.go

f7115f4

Co-authored-by: Oleg Zaytsev <[email protected]>

take config before locking tracker map

679f2cc

simplify logics

66accc9

remove useless initialization

f4a4efd

change int64 to time.x

f90ac0e

change pointer to instance

1ab89c5

change instance to pointer in map

23b32cf

remove callback

7a60c7d

use string when create new key in map

0287bf6

move the logic to different place

9c4c2df

get cat once out of loop

f8f2a49

update tracker per request for received samples

1ad99ad

make the lock fanny by dum dum

fa62ee1

make ingester work

1b0fb00

fix lock

ced8346

ying-jeanne commented Dec 31, 2024

View reviewed changes

ying-jeanne added 2 commits December 31, 2024 10:09

add changelog

0a7c858

update changelog

4336f7f

ying-jeanne requested a review from colega December 31, 2024 09:13

ying-jeanne added 3 commits December 31, 2024 10:36

update doc with correct metrics name

67b6cea

remove useless function

800fe85

cast only once

80e69fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MVP: Cost attribution #10269

MVP: Cost attribution #10269

ying-jeanne commented Dec 17, 2024 •

edited

Loading

colega Dec 18, 2024

ying-jeanne Dec 24, 2024

colega Dec 18, 2024

ying-jeanne Dec 26, 2024

colega Dec 18, 2024

ying-jeanne Dec 24, 2024

ying-jeanne Dec 24, 2024

colega Dec 18, 2024

ying-jeanne Dec 27, 2024

tacole02 left a comment

colega Dec 27, 2024

ying-jeanne Dec 30, 2024

colega Dec 27, 2024

ying-jeanne Dec 30, 2024

colega Dec 27, 2024

ying-jeanne Dec 30, 2024

colega Dec 27, 2024

ying-jeanne Dec 30, 2024

ying-jeanne Dec 31, 2024 •

edited

Loading

		replace github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20241219104229-b50052711673
		replace github.com/prometheus/prometheus => github.com/grafana/mimir-prometheus v0.0.0-20241224134504-460b7be5bce8

MVP: Cost attribution #10269

Are you sure you want to change the base?

MVP: Cost attribution #10269

Conversation

ying-jeanne commented Dec 17, 2024 • edited Loading

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacole02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ying-jeanne Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

ying-jeanne commented Dec 17, 2024 •

edited

Loading

ying-jeanne Dec 31, 2024 •

edited

Loading